Skip to content

ext/soap: Fix integer overflow when decoding SOAP array indexes#21963

Open
LamentXU123 wants to merge 7 commits intophp:masterfrom
LamentXU123:sec-fix
Open

ext/soap: Fix integer overflow when decoding SOAP array indexes#21963
LamentXU123 wants to merge 7 commits intophp:masterfrom
LamentXU123:sec-fix

Conversation

@LamentXU123
Copy link
Copy Markdown
Contributor

@LamentXU123 LamentXU123 commented May 6, 2026

SOAP encoded array metadata such as arrayType, offset, and position is parsed into int indexes before being used for array placement. That is, this code:

<?php
class C extends SoapClient {
    function __doRequest($r, $l, $a, $v, $o = false, ?string $u = null): string {
        return <<<'XML'
<SOAP-ENV:Envelope xmlns:SOAP-ENV="http://schemas.xmlsoap.org/soap/envelope/"
 xmlns:SOAP-ENC="http://schemas.xmlsoap.org/soap/encoding/"
 xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
 xmlns:xsd="http://www.w3.org/2001/XMLSchema"
 SOAP-ENV:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/">
 <SOAP-ENV:Body>
  <x:testResponse xmlns:x="x">
   <return SOAP-ENC:arrayType="xsd:string[1]" SOAP-ENC:offset="[2147483648]" xsi:type="SOAP-ENC:Array">
    <item xsi:type="xsd:string">x</item>
   </return>
  </x:testResponse>
 </SOAP-ENV:Body>
</SOAP-ENV:Envelope>
XML;
    }
}

$c = new C(null, ['location' => 'x', 'uri' => 'x']);
var_dump($c->test());

Will resulted in:

array(1) {
  [-2147483648]=>
  string(1) "x"
}

Because the index overflows.

There is a TODO message to fix this. So I think people are aware of this bug before

I therefore wrote this to add checked decimal accumulation for SOAP array indexes and reject values that exceed int range. I Also reject auto-increment past INT_MAX before updating the next array position :)

@LamentXU123 LamentXU123 requested a review from devnexen as a code owner May 6, 2026 14:54
@LamentXU123 LamentXU123 changed the title ext/soap: Fix SOAP array index integer overflow ext/soap: Fix integer overflow when decoding SOAP array indexes May 6, 2026
@LamentXU123
Copy link
Copy Markdown
Contributor Author

LamentXU123 commented May 6, 2026 •
edited
Loading

Plus: Had to fix the bailout logic....

Now in the client side, if I raise a error when overflow is detected, the code will go to master_to_eval() and without xmlFreeDoc(response) and cause a memory leak. So in case of that I just had to add some logic to the bailout process to make sure xmldoc is free, although I hate it (；′⌒`)

devnexen
devnexen reviewed May 6, 2026
View reviewed changes
Comment thread ext/soap/php_encoding.c Outdated
devnexen
devnexen reviewed May 7, 2026
View reviewed changes
Comment thread ext/soap/php_packet_soap.c
@devnexen
Copy link
Copy Markdown
Member

devnexen commented May 7, 2026

looks good, but I ll have another look more toward the end of this week (and likely merging it in the process). Thanks for working on it !

devnexen
devnexen reviewed May 9, 2026
View reviewed changes
Comment thread ext/soap/php_encoding.c
Comment thread ext/soap/php_encoding.c
Comment thread ext/soap/tests/soap_array_index_overflow.phpt
@devnexen
Copy link
Copy Markdown
Member

devnexen commented May 9, 2026

as mentioned, overall good just few minor nits to address !

@LamentXU123
Copy link
Copy Markdown
Contributor Author

LamentXU123 commented May 9, 2026 •
edited
Loading

as mentioned, overall good just few minor nits to address !

Thanks for reviewing! The fix should be in place in a couple of minutes.

Now I think this is rather a bug fix so it is more proper to rebase this on 8.4 I don't sure if this should be counted as a minor security fix(signed integer overflow) if than this should go to 8.2

@devnexen
Copy link
Copy Markdown
Member

devnexen commented May 9, 2026 •
edited
Loading

I m not sure it would qualify as actual security fix for 8.2 but let me discuss it internally

@LamentXU123
Copy link
Copy Markdown
Contributor Author

LamentXU123 commented May 9, 2026 •
edited
Loading

I m not sure it would qualify as actual security fix for 8.2 but let me discuss it internally

Take your time :) I am first going to rebase this on 8.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devnexen devnexen devnexen left review comments

Assignees

No one assigned

Labels

Extension: soap

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@LamentXU123 @devnexen